-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove zip317 gate #1184
Remove zip317 gate #1184
Conversation
…' into remove_zip317_gate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand, in my mind there are two options:
- keep do_send and rename it to old_send (or legacy_send?)
- delete do_send and then there is no such thing as old_send
it doesnt make sense that this PR does half the work to switch over to zip317 but i will approve (when the tests all pass ofc) on the basis we create an issue that is a blocker on release that makes sure we do the rest of the work to clean up the tests to have the correct inputs/outputs for quick_send and delete old_send
@@ -3,9 +3,6 @@ use std::fmt; | |||
#[derive(Debug)] | |||
pub(crate) enum CommandError { | |||
ArgsNotJson(json::Error), | |||
#[cfg(feature = "zip317")] | |||
#[allow(dead_code)] | |||
ArgNotJsonOrValidAddress, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is needed for send_all please dont remove as its going to land now
InvalidPool, | ||
#[cfg(feature = "zip317")] | ||
#[allow(dead_code)] | ||
MultipleReceivers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is needed for send_all please dont remove as its going to land now
ArgNotJsonOrValidAddress => write!( | ||
f, | ||
"argument cannot be converted to a valid address or parsed as json." | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is needed for send_all please dont remove as its going to land now
#[cfg(not(feature = "zip317"))] | ||
InvalidPool => write!(f, "invalid pool."), | ||
#[cfg(feature = "zip317")] | ||
MultipleReceivers => write!(f, "'send all' can only accept one receiver."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is needed for send_all please dont remove as its going to land now
I believe this PR is a good place to start updating integration tests.
If I mark this as ready for review, it will run CI, I think we'd like to see CI pass.